-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: expose store options in PiniaCustomProperties for plugin access #3042
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v3
Are you sure you want to change the base?
feat: expose store options in PiniaCustomProperties for plugin access #3042
Conversation
- Add StoreOptionsAccess utility type for accessing custom store options - Modify store creation to include _options property with store options - Export StoreOptionsAccess type from main module - Add comprehensive tests for plugin access to custom store options - Support both option stores and setup stores - Maintain backward compatibility with existing plugins Fixes vuejs#1247
✅ Deploy Preview for pinia-official canceled.
|
WalkthroughExposes per-store defineStore options at runtime via a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Pinia
participant Store
participant Plugin
App->>Pinia: createPinia()
App->>Pinia: pinia.use(Plugin)
App->>Pinia: defineStore(options) %% options include customOption / stores / debounce
App->>Store: useMainStore(pinia)
Note right of Store #D3E4CD: createSetupStore constructs instance\nand assigns _options = optionsForPlugin
Pinia-->>Plugin: initialize plugin with { store, options }
Plugin->>Store: read context.options / store._options
Plugin-->>App: expose computed getters based on options
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/pinia/src/store.ts (2)
471-489: Store options should be non-reactive to avoid needless tracking
_optionscurrently references a reactive object. Mark it raw when storing on the reactivestoreto avoid proxying user options and functions.const store: Store<Id, S, G, A> = reactive( __DEV__ || (__USE_DEVTOOLS__ && IS_CLIENT) ? assign( { _hmrPayload, _customProperties: markRaw(new Set<string>()), // devtools custom properties - _options: optionsForPlugin, // store options for plugins + _options: markRaw(optionsForPlugin), // store options for plugins }, partialStore ) : assign( { - _options: optionsForPlugin, // store options for plugins + _options: markRaw(optionsForPlugin), // store options for plugins }, partialStore ) ) as unknown as Store<Id, S, G, A>
682-701: Hide_optionsfrom devtools/object enumerationInternal fields are made non-enumerable for devtools;
_optionsshould be included to prevent leaking internal shapes.- ;(['_p', '_hmrPayload', '_getters', '_customProperties'] as const).forEach( + ;(['_p', '_hmrPayload', '_getters', '_customProperties', '_options'] as const).forEach( (p) => { Object.defineProperty( store, p, assign({ value: store[p] }, nonEnumerable) ) } )
🧹 Nitpick comments (4)
packages/pinia/src/types.ts (1)
277-284: Avoidanyon internal_optionsTyping
_optionsasanyleaks unsoundness into the public store type. Preferunknown(safer), since plugins should access options viacontext.optionsand type utilities, not through_options.- _options?: any + _options?: unknownpackages/pinia/src/store.ts (1)
592-679: HMR: keep_optionsin syncDuring HMR,
_optionsis not updated, so plugins readingstore._optionswill see stale values in dev. Copy it fromnewStore.// update the values used in devtools and to allow deleting new properties later on store._hmrPayload = newStore._hmrPayload store._getters = newStore._getters + // keep plugin options in sync during HMR + // @ts-expect-error: internal field + store._options = newStore._options store._hotUpdating = falsepackages/pinia/__tests__/storeOptionsAccess.spec.ts (2)
2-2: ImportStoreOptionsAccessfor stronger runtime test typingsUse the new utility in the module augmentation to validate types here as well (mirrors the dts test).
-import { createPinia, defineStore, StoreDefinition } from '../src' +import { createPinia, defineStore, StoreDefinition, StoreOptionsAccess } from '../src'
14-18: UseStoreOptionsAccessin augmentation instead ofanyThis exercise ensures the new utility works in real tests, not only in dts.
export interface PiniaCustomProperties<Id, S, G, A> { - readonly stores: any - readonly customOption: any - readonly debounce: any + readonly stores: StoreOptionsAccess<this, 'stores'> + readonly customOption: StoreOptionsAccess<this, 'customOption'> + readonly debounce: StoreOptionsAccess<this, 'debounce'> }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/pinia/__tests__/storeOptionsAccess.spec.ts(1 hunks)packages/pinia/src/index.ts(1 hunks)packages/pinia/src/store.ts(1 hunks)packages/pinia/src/types.ts(2 hunks)packages/pinia/test-dts/storeOptionsAccess.test-d.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/pinia/src/types.ts (1)
packages/pinia/src/index.ts (2)
StoreOptionsAccess(31-31)Store(18-18)
packages/pinia/__tests__/storeOptionsAccess.spec.ts (2)
packages/pinia/src/types.ts (4)
DefineStoreOptionsBase(635-635)Store(473-485)StoreDefinition(502-527)PiniaCustomProperties(532-537)packages/pinia/src/store.ts (1)
defineStore(844-939)
packages/pinia/test-dts/storeOptionsAccess.test-d.ts (2)
packages/pinia/__tests__/storeOptionsAccess.spec.ts (2)
DefineStoreOptionsBase(8-12)PiniaCustomProperties(14-18)packages/pinia/src/types.ts (5)
DefineStoreOptionsBase(635-635)Store(473-485)StoreDefinition(502-527)PiniaCustomProperties(532-537)StoreOptionsAccess(556-556)
🔇 Additional comments (2)
packages/pinia/src/index.ts (1)
31-31: LGTM: new public type exportRe-exporting
StoreOptionsAccessfrom./typeslooks correct and non-breaking.packages/pinia/test-dts/storeOptionsAccess.test-d.ts (1)
1-24: LGTM: dts coverage is solidAugmentation and
StoreOptionsAccessusage validate the intended typing surface.
| /** | ||
| * Utility type to access store options within PiniaCustomProperties. | ||
| * This allows plugins to access custom options defined in DefineStoreOptionsBase. | ||
| * | ||
| * @example | ||
| * ```ts | ||
| * declare module 'pinia' { | ||
| * export interface DefineStoreOptionsBase<S, Store> { | ||
| * stores?: Record<string, StoreDefinition>; | ||
| * } | ||
| * | ||
| * export interface PiniaCustomProperties<Id, S, G, A> { | ||
| * readonly stores: any; // Use any for now, will be properly typed by plugins | ||
| * } | ||
| * } | ||
| * ``` | ||
| */ | ||
| export type StoreOptionsAccess<Store, Key extends keyof any> = any | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement StoreOptionsAccess to actually resolve plugin-augmented option types
Right now it’s any, defeating the PR goal. Make it map a Store instance to its DefineStoreOptionsBase augmentation and pick Key, falling back to undefined when absent.
-export type StoreOptionsAccess<Store, Key extends keyof any> = any
+/**
+ * Resolves the type of a custom option declared via `DefineStoreOptionsBase`
+ * for the given Store instance type and option `Key`.
+ * Returns `undefined` if the option is not declared.
+ */
+export type StoreOptionsAccess<
+ This,
+ Key extends keyof any,
+ Fallback = undefined,
+> =
+ This extends Store<infer Id, infer S, infer G, infer A>
+ ? DefineStoreOptionsBase<S, Store<Id, S, G, A>> extends infer O
+ ? O extends Record<PropertyKey, unknown>
+ ? Key extends keyof O
+ ? O[Key]
+ : Fallback
+ : Fallback
+ : Fallback
+ : Fallback📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Utility type to access store options within PiniaCustomProperties. | |
| * This allows plugins to access custom options defined in DefineStoreOptionsBase. | |
| * | |
| * @example | |
| * ```ts | |
| * declare module 'pinia' { | |
| * export interface DefineStoreOptionsBase<S, Store> { | |
| * stores?: Record<string, StoreDefinition>; | |
| * } | |
| * | |
| * export interface PiniaCustomProperties<Id, S, G, A> { | |
| * readonly stores: any; // Use any for now, will be properly typed by plugins | |
| * } | |
| * } | |
| * ``` | |
| */ | |
| export type StoreOptionsAccess<Store, Key extends keyof any> = any | |
| /** | |
| * Utility type to access store options within PiniaCustomProperties. | |
| * This allows plugins to access custom options defined in DefineStoreOptionsBase. | |
| * | |
| * @example | |
| * |
🤖 Prompt for AI Agents
In packages/pinia/src/types.ts around lines 539 to 557, StoreOptionsAccess is
currently typed as any; replace it with a conditional/lookup type that, given a
Store instance type, resolves the corresponding augmented DefineStoreOptionsBase
for that store and picks the property Key (returning that property type or
undefined if missing). Implement this using TypeScript conditional/lookup types
that extract the appropriate DefineStoreOptionsBase augmentation for the
provided Store type (e.g., by inferring the store identifier or matching the
Store instance type to the augmented interface) and return the property at Key
or undefined as a fallback.
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
…operties` Docstrings generation was requested by @doubledare704. * #3042 (comment) The following files were modified: * `packages/pinia/src/store.ts`
|
Note Generated docstrings for this pull request at #3044 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/pinia/src/store.ts (3)
487-505: Make_optionsnon-reactive to avoid devtools/serialization churn.Right now
_optionsbecomes reactive (proxied) and enumerable. This can:
- Surface large, non-serializable trees/functions in devtools and snapshots.
- Add needless proxy work.
- Allow accidental mutation by plugins/userland.
Minimal safe change: mark it raw.
? assign( { _hmrPayload, _customProperties: markRaw(new Set<string>()), // devtools custom properties - _options: optionsForPlugin, // store options for plugins + _options: markRaw(optionsForPlugin), // store options for plugins }, partialStore ) : assign( { - _options: optionsForPlugin, // store options for plugins + _options: markRaw(optionsForPlugin), // store options for plugins }, partialStore )If you want the stronger guarantee (recommended): define
_optionsas non-enumerable and readonly right after store creation instead of including it in theassign()object.// place right after the reactive(store) creation, once optionsForPlugin is fully prepared Object.defineProperty(store, '_options', { value: markRaw(optionsForPlugin), enumerable: false, configurable: true, // allow HMR to redefine writable: false, })
706-716: Hide_optionsfrom devtools property listing.For consistency with other internals and to avoid noisy/unsafe serialization, include
_optionsin the non-enumerable set.- ;(['_p', '_hmrPayload', '_getters', '_customProperties'] as const).forEach( + ;(['_p', '_hmrPayload', '_getters', '_customProperties', '_options'] as const).forEach(
609-695: Sync _options during HMRHMR copies actions/getters/state but not store._options, so plugins reading per-store options after HMR will see stale values — update _options near the end of store._hotUpdate (before updating _hmrPayload/_getters):
// sync plugin options if ('_options' in newStore) { Object.defineProperty(store, '_options', { value: newStore._options, enumerable: false, configurable: true, writable: false, }) }File: packages/pinia/src/store.ts — inside store._hotUpdate (approx. lines 609–695).
🧹 Nitpick comments (1)
packages/pinia/src/store.ts (1)
216-231: Docblock LGTM (tiny nits optional).Reads clearly and matches the function behavior. If you want, tighten wording: “Returns a reactive store instance that exposes …” instead of “The reactive Store instance is exposing …”.
- Update docstring for `createSetupStore` return type for clarity - Mark `_options` as raw to prevent reactivity issues - Add syncing of `_options` property during HMR - Ensure `_options` is non-enumerable and consistent in internal properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/pinia/src/store.ts (4)
216-231: Good docblock; consider markingisOptionsStoreas internal.To avoid implying public support, add an
@internalnote forisOptionsStorein the TSDoc.
493-493: Avoid enumerating_optionson the public store in non-devtools builds.Inline
assign(..., { _options: ... })makes_optionsenumerable (SSR/plugins iterating keys will see it). Prefer defining it as non-enumerable right after store creation.Apply this diff to stop setting
_optionshere:- _customProperties: markRaw(new Set<string>()), // devtools custom properties - _options: markRaw(optionsForPlugin), // store options for plugins + _customProperties: markRaw(new Set<string>()), // devtools custom propertiesThen, right after the reactive store is created (below Line 505), add:
Object.defineProperty(store as any, '_options', { value: markRaw(optionsForPlugin), enumerable: false, configurable: true, writable: true, // keep parity with other internals; see devtools block })
499-504: Same enumerability concern for non-devtools path.Make
_optionsnon-enumerable at definition time for consistency with the devtools path and to avoid leaking into key iterations.Apply this diff:
- : assign( - { - _options: markRaw(optionsForPlugin), // store options for plugins - }, - partialStore - ) + : assign({}, partialStore)(Then keep the single
Object.definePropertyaddition suggested above to define_optionsonce.)
691-699: HMR redefines_optionswith different writability/enumerability; preserve prior descriptor.Currently HMR sets
writable: falseandenumerable: false, which may diverge from initial flags and surprise plugins. Reuse the existing descriptor’s flags.Apply this diff:
- if ('_options' in newStore) { - Object.defineProperty(store, '_options', { - value: newStore._options, - enumerable: false, - configurable: true, - writable: false, - }) - } + if ('_options' in newStore) { + const prev = Object.getOwnPropertyDescriptor(store, '_options') + Object.defineProperty(store, '_options', { + value: newStore._options, + enumerable: prev?.enumerable ?? false, + configurable: true, + writable: prev?.writable ?? true, + }) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/pinia/src/store.ts(4 hunks)
🔇 Additional comments (1)
packages/pinia/src/store.ts (1)
716-731: Hiding_optionsfrom devtools is correct — verify locally.packages/pinia/src/store.ts (≈lines 700–740) shows '_options' included in the non-enumerable list; keeping it non-enumerable is appropriate. Sandbox could not run workspace tests (pnpm test failed due to missing dev tooling/node_modules). Run locally and confirm no regressions and no unintended writes to _options:
pnpm -w install
pnpm -w test
pnpm -w -r test-dts
rg -n --hidden -S '._options\s*=' -g '!/node_modules/' -C2
Fixes #1247
Summary by CodeRabbit
New Features
Tests